Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable zarr backend testing in data tests [1] #1056

Merged
merged 12 commits into from
Sep 13, 2024
Merged

Enable zarr backend testing in data tests [1] #1056

merged 12 commits into from
Sep 13, 2024

Conversation

h-mayorquin
Copy link
Collaborator

This enables zarr testing for running conversions with backend specified as a string (still missing the building a backend configuration part)

It also makes some metadata checks proper tests on their own which has two advantages:

  • They will fail on their own and be identified as such by pytest
  • Can be parallelized with xdist.

@h-mayorquin h-mayorquin self-assigned this Sep 6, 2024
@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Sep 6, 2024 •
edited
Loading

@CodyCBakerPhD OK, we got the point were the failures are actually related to zarr, check out the video test failure:

https://github.com/catalystneuro/neuroconv/actions/runs/10744684959/job/29802201096?pr=1056

Does it ring any bell? I will dig into it but is a windows only error so it will have to wait a bit for me.

@h-mayorquin h-mayorquin mentioned this pull request Sep 6, 2024
Closed
@h-mayorquin
Copy link
Collaborator Author

Note for myself to also add the documentation:

#1019

@CodyCBakerPhD
Copy link
Member

Don't believe I've seen this one myself, but from the message the path to the Zarr array (file): https://github.com/catalystneuro/neuroconv/actions/runs/10744684959/job/29802201096?pr=1056#step:31:3426

is not a valid Windows path (should have replaced the / with \\?)

@h-mayorquin
Copy link
Collaborator Author

@CodyCBakerPhD
OK, this should be ready but it requires the fix on #1070 before (as does everything else).

The cause of the Windows error is here:

hdmf-dev/hdmf-zarr#219

@h-mayorquin h-mayorquin mentioned this pull request Sep 11, 2024
Open
@h-mayorquin h-mayorquin changed the title Enable zarr backend testing in interface testing [1] Enable zarr backend testing in data tests [1] Sep 11, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review September 12, 2024 15:19
@h-mayorquin
Copy link
Collaborator Author

@CodyCBakerPhD this is ready to go.

@CodyCBakerPhD
Copy link
Member

Coolio

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) September 12, 2024 15:52
@CodyCBakerPhD CodyCBakerPhD merged commit 96c8ed4 into main Sep 13, 2024
35 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the more_granular_tests branch September 13, 2024 07:00
@h-mayorquin h-mayorquin mentioned this pull request Sep 14, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.40%. Comparing base (81a022d) to head (8590f2f).
Report is 66 commits behind head on main.

Files with missing lines Patch % Lines
...c/neuroconv/tools/testing/data_interface_mixins.py 97.67% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1056      +/-   ##
==========================================
+ Coverage   90.32%   90.40%   +0.08%     
==========================================
  Files         129      129              
  Lines        7996     8029      +33     
==========================================
+ Hits         7222     7259      +37     
+ Misses        774      770       -4
Flag Coverage Δ
unittests 90.40% <97.67%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...atainterfaces/behavior/video/videodatainterface.py 93.63% <ø> (ø)
...c/neuroconv/tools/testing/data_interface_mixins.py 95.47% <97.67%> (+0.14%) ⬆️

... and 4 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodyCBakerPhD CodyCBakerPhD CodyCBakerPhD approved these changes

Assignees

@h-mayorquin h-mayorquin

Labels
testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@h-mayorquin @CodyCBakerPhD